feat(elements): Add support for sign in with passkey#3472
feat(elements): Add support for sign in with passkey#3472panteliselef merged 24 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: d16809f The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| case 'passkey': { | ||
| return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey(); | ||
| } |
There was a problem hiding this comment.
When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey
| const [isSupported, setIsSupported] = React.useState(false); | ||
| React.useEffect(() => { | ||
| async function runAutofillPasskey() { | ||
| const _isSupported = await isWebAuthnAutofillSupported().catch(() => false); | ||
| setIsSupported(_isSupported); | ||
| } | ||
|
|
||
| // @ts-expect-error - Depending on type the props can be different | ||
| if (passthroughProps?.passkeyAutofill) { | ||
| runAutofillPasskey(); | ||
| } | ||
|
|
||
| // @ts-expect-error - Depending on type the props can be different | ||
| }, [passthroughProps?.passkeyAutofill]); |
There was a problem hiding this comment.
Should this be handled inside an actor ?
There was a problem hiding this comment.
Which part are you referring to?
There was a problem hiding this comment.
Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.
Why does passkeyAutofill need to be on the input?
There was a problem hiding this comment.
This is only necessary if we want to support passkey autofill.
There was a problem hiding this comment.
Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.
There was a problem hiding this comment.
We also need to call authenticateWithPasskey internally
| on: { | ||
| 'AUTHENTICATE.PASSKEY': { | ||
| guard: not('isExampleMode'), | ||
| target: 'AttemptingPasskey', | ||
| reenter: true, | ||
| }, | ||
| SUBMIT: { | ||
| guard: not('isExampleMode'), | ||
| target: 'Attempting', | ||
| reenter: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.
brkalow
left a comment
There was a problem hiding this comment.
Looking good. I'd like to see the passkey specific logic moved out of the input logic.
| const [isSupported, setIsSupported] = React.useState(false); | ||
| React.useEffect(() => { | ||
| async function runAutofillPasskey() { | ||
| const _isSupported = await isWebAuthnAutofillSupported().catch(() => false); | ||
| setIsSupported(_isSupported); | ||
| } | ||
|
|
||
| // @ts-expect-error - Depending on type the props can be different | ||
| if (passthroughProps?.passkeyAutofill) { | ||
| runAutofillPasskey(); | ||
| } | ||
|
|
||
| // @ts-expect-error - Depending on type the props can be different | ||
| }, [passthroughProps?.passkeyAutofill]); |
There was a problem hiding this comment.
Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.
Why does passkeyAutofill need to be on the input?
fb140bc to
4737f40
Compare
brkalow
left a comment
There was a problem hiding this comment.
Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.
Any ideas for not referencing the sign in flow actor directly in the common input components?
| React.useEffect(() => { | ||
| if (passkeyAutofillSupported) { | ||
| signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' }); | ||
| } | ||
| }, [passkeyAutofillSupported, signInRouterRef]); |
There was a problem hiding this comment.
Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic
There was a problem hiding this comment.
Probably yes, but if felt like something only this component should care about.
because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.
There was a problem hiding this comment.
Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.
| (props: FormInputProps, forwardedRef) => { | ||
| const clerk = useClerk(); | ||
| const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill; | ||
| const signInRouterRef = SignInRouterCtx.useActorRef(true); |
There was a problem hiding this comment.
I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔
Maybe @tmilewski has an idea?
There was a problem hiding this comment.
Is this better ?
<SignIn.Passkey>
<Clerk.Input />
</SignIn.Passkey>There was a problem hiding this comment.
I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.
Another thought is to have a SignIn.PasskeyInput, but this does not feel right.
There was a problem hiding this comment.
We are now simply detecting autoComplete="webauthn"
| const field = useInput(props); | ||
|
|
||
| const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn')); | ||
| const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type); |
There was a problem hiding this comment.
Should we consider tel here for phone numbers?
There was a problem hiding this comment.
I think you are correct
Description
This PR add support for passkey usage within a SignIn flow
APIs introduced:
<SignIn.Passkey /><SignIn.SupportedStrategy name='passkey'><SignIn.Strategy name='passkey'><Clerk.Input type='text' passkeyAutofill>Usage Examples:
<SignIn.Passkey /><SignIn.SupportedStrategy name='passkey'><SignIn.Strategy name='passkey'><Clerk.Input type='text' passkeyAutofill>Docs PR: clerk/clerk-docs#1132
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change